Skip to content

3D Touch Preview API#2186

Merged
guyca merged 19 commits into
wix:masterfrom
birkir:feature/preview-peek-pop
Nov 23, 2017
Merged

3D Touch Preview API#2186
guyca merged 19 commits into
wix:masterfrom
birkir:feature/preview-peek-pop

Conversation

@birkir
Copy link
Copy Markdown
Contributor

@birkir birkir commented Nov 19, 2017

Added the preview and pop feature.

It is currently implemented so the javascript API is untouched (uses passProps) but I would like to see if anyone has interest in this feature so we can give it some API love.

To use the feature you simply call navigator.push() but with additional parameter previewViewID which is a React Node.

<TouchableOpacity
  onPressIn={() => this.props.navigator.push({
    screen: 'screenid',
    previewView: this.viewRef,
    previewActions: [{
      title: 'Foo',
      style: 'selected', // none, selected, destructive
      actions: [{ title: 'Bar' }],
    }],
 })
/>

previewandpop

Going forward with this would include some of the next steps:

  • Move previewViewID from passProps to it's parent.
  • Added some fail-safe wrappers around the added class methods
  • Added preview actions (with groups).
  • Moved away from passProps to actionParams.
  • Added action navigator events.
  • Conditionally commit to pop on deep press.
  • Option to set explicit height for the preview on shallow press.
  • Documentation

@messense
Copy link
Copy Markdown

This is awesome!

Copy link
Copy Markdown
Contributor

@eliperkins eliperkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, as I understand it, we need to have already rendered our previewView somewhere in React, since we need to pass along the node handle in passProps? How will this work with cases where we want to preview a different view than one we already have rendered (say, a new view controller)?

Comment thread ios/RCCViewController.h Outdated
- (void)setStyleOnInit;
- (void)updateStyle;
- (void)setNavBarVisibilityChange:(BOOL)animated;
- (void)setPreviewController:(RCCViewController *)controller;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be implemented as a @property, rather than explicit getters and setters with private ivars.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment thread ios/RCCNavigationController.m Outdated

NSString *previewViewID = passProps[@"previewViewID"];
if (previewViewID) {
RCCViewController *topViewController = ((RCCViewController*)self.topViewController);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a little scary. Do we have a way to guarantee that this view controller will be a RCCViewController? Perhaps we should wrap this cast with a check to see if the view controller responds to the specific selector that we're calling on it (@selector(setPreviewController:)).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right, in the if(keepStyleAcrossPushBool) statement above it is actually doing an if ([self.topViewController isKindOfClass:[RCCViewController class]]).

I will incorporate this, which one would you prefer?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either isKindOfClass or respondsToSelector is fine with me!

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 20, 2017

So, as I understand it, we need to have already rendered our previewView somewhere in React, since we need to pass along the node handle in passProps? How will this work with cases where we want to preview a different view than one we already have rendered (say, a new view controller)?

Thank you for the feedback!

  1. previewViewID is actually a react view that will have the 3D touch enabled on it. When you press it it will blur the background out (Image 1)
  2. When you press it harder, it will show the view controller given in the push configuration (Image 2)
  3. And if you press even harder, it will "pop" the view controller as a newly pushed screen (Image 3)

sample

I like to think of it as a different way to navigator.push.

@brunolemos
Copy link
Copy Markdown

Awesome!

API love would be great.
Ideally it should receive any normal component instead of an id, no?
But I can imagine it could lag if it wasn't already rendered.

Also add support for action buttons:

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 21, 2017

Yes, that is what the Preview component would just do behind the scene (get the ref etc.)

It must be rendered before the force touch is started.

Actions is good and easy addon, but providing extensive api for it would require some work (like click action and show another set of actions)

Example of this is in the Apollo app, force press an item, click share, and you will be promted with another set of actions (share image / share link to image)

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 22, 2017

  • Added some fail-safe wrappers around the added class methods
  • Added preview actions (with groups).
  • Moved away from passProps to actionParams.
  • Added action navigator events.

I am actually thinking about moving away from providing specific component that will act as previewViewID. The preview view component is only used to create and measure a rectangle on the screen to not blur on force press. We could remove the need for providing the actual react id and just provide the ref by handling findNodeHandle internally.

What do you think?

@messense
Copy link
Copy Markdown

+1 on handling findNodeHandle internally, findNodehandle API might not be as well known as ref.

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 22, 2017

Ok. Then I think we are good to go, at least to begin with. We could iterate on this if we get some feedback from people.

Link

const screenInstanceID = _.uniqueId('screenInstanceID');
if (params.previewView instanceof Component) {
previewViewID = findNodeHandle(params.previewView)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should have an else branch to treat previewView as previewViewID directly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea, thanks!

@brunolemos
Copy link
Copy Markdown

brunolemos commented Nov 22, 2017

Looking great!

Did you test this two cases:

  • The preview component will probably be a View hidden in the screen, right? Not like the example that you used the visible "Row". How should the user proceed? Hide it's parent with height: 0 or what?
  • On top of the example above, did you test with dynamic content? E.g. If I force touch Row1 it should show Photo1 and if I force touch Row2 it should show Photo2 inside and reuse the same preview component

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 22, 2017

  1. I don't think it will. The previewView will always be something already visible on the screen (like a Row or a picture cell like in the GIF's above).

  2. The content of the preview is always the screen you defined in the navigator.push (with all its settings). The screen will be immediately rendered in the background and transitioned into an preview based on the force of your touch.

With our current implementation we can replicate every app I've seen yet that has this feature.

Only limitation is the preview height (Like the image preview), but once we implement showLightbox, the preview controller should be able to use that height.

Also we need to implement a configurable boolean flag to skip the actual push on hard press.

Does this make sense to you?

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 22, 2017

Ok after reading your second point again I realized that this could actually be done, but as to my understanding we don't have any easy way to update props of an RCCViewController yet?

But it should be easy to check if the previewController is is the same as the proposed one and just re-use it and update its styles and props.

Comment thread ios/RCCViewController.m Outdated
}

- (void)previewingContext:(id<UIViewControllerPreviewing>)previewingContext commitViewController:(UIViewController *)viewControllerToCommit {
[self.navigationController pushViewController:self.previewController animated:false];
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we may want to provide the user to skip this operation if he doesn't want the screen to be pushed.

@brunolemos
Copy link
Copy Markdown

brunolemos commented Nov 22, 2017

The previewView will always be something already visible on the screen

Ok sorry, I had misunderstood the API. I thought previewView was the small preview view that shows while you are force touching. But it seems to be the component that receives the touch, right?

So I'll rephrase my question. Let's suppose this use case:

While you force touch on Instagram (peek), it shows a minimal preview screen with just 3 components: the profile photo, the profile name and the photo. When you finish force touching (pop), it shows the full page with all other contents (follow button, like button, etc).

Is this case supported?
By your example and code I get the idea that will always show the full page, just without the navigation bar.

If not, one possible solution is to pass a prop to the screen saying if it's on the peek/preview state or not. Another way would be by using events (might be boring to always have to listen to it and set the state accordingly, on the other hand it's more flexible as the prop name is not fixed).

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 22, 2017

Gotcha! No problem.

We can pass a prop saying it's a preview and then send ScreenChanged event once it's pushed (didPreview etc.)

For the instagram case, what is really happening is just cropping + conditional rendering. How they do the height thing, IDK.

Maybe it would just work by setting fixed height to the first View inside the screen. I will verify and come back to you.

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 22, 2017

Allright guys, I have tested all cases discussed in the thread. I added the option conditionally commit to pop the controller and to set explicit height to the preview with the recommended preferredContentSize suggested by Apple.

Please let me know if you have any more concerns about this PR, I'd like to get it merged to get some feedback from real users and see if anybody will find an limitation with the current implementation.

@rgoldiez
Copy link
Copy Markdown
Contributor

@birkir - This is really awesome. Thank you for your work on this. However, like @enahum I'm seeing the onPress fire when the preview shows and then is dismissed from the onPressIn. I suppose this is expected since onPressIn fires, followed by onPress, followed by onPressOut. I'm using onLongPress for now and it seems to work well!

@rgoldiez
Copy link
Copy Markdown
Contributor

@birkir - Does the previewView need to be an instance of a Component? I get an error if referencing a View? So, in the case where I wanted to reference a View, I simply changed my component from a View to an Animated.View (even though I'm not animating it) and it worked just fine.

@rgoldiez
Copy link
Copy Markdown
Contributor

@birkir - so I’ve been playing around with this new functionality and it’s super easy to use. One thing that would be nice to have is a prop that goes true when a preview has been committed. I’m currently listening for the event, which works as expected, but if I want to conditionally block rendering of a component when in preview mode, I need to tract state for when a preview gets committed. Not a huge deal but this could be a small improvement that makes use even easier.

@brunolemos
Copy link
Copy Markdown

One thing that would be nice to have is a prop that goes true when a preview has been committed. I’m currently listening for the event, which works as expected, but if I want to conditionally block rendering of a component when in preview mode, I need to tract state for when a preview gets committed

Yep that's exactly what I meant with:

pass a prop to the screen saying if it's on the peek/preview state or not. Another way would be by using events (might be boring to always have to listen to it and set the state accordingly

@rgoldiez
Copy link
Copy Markdown
Contributor

@brunolemos - Can you explain what you mean?

@brunolemos
Copy link
Copy Markdown

@rgoldiez I meant: "+1"

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 27, 2017

Cool thanks for the feedback guys. Can we update screen props somehow dynamically? I don't think this is done anywhere in RNN at the moment.

I know it's a hassle to add a listener and a state mechanism, but it's very common pattern with RNN in general (didAppear+didDisappear).

thanhzusu pushed a commit to thanhzusu/react-native-navigation that referenced this pull request Nov 27, 2017
* master: (22 commits)
  setTabButton to change the label of the TabButton (wix#2215)
  3D Touch Preview API (wix#2186)
  Update android-specific-use-cases.md
  Update android-specific-use-cases.md
  Load props from props registry for redux screens
  Nav bar save props (wix#2211)
  Support passing unserializable props to custom navBar component
  Save buttons props for root screens
  Ensure that styles set on individual buttons are not overridden (wix#2200)
  Update top-level-api.md
  (Android) Allow disableOpenGesture right or left in the drawer (wix#2189)
  Revert "Support iOS 11 prefersLargeTitles #1643 (wix#2090)" (wix#2204)
  Support iOS 11 prefersLargeTitles #1643 (wix#2090)
  Support passing unserializable props to custom button (wix#2192)
  addOnNavigatorEvent improvements (wix#2175)
  Fix missing props (wix#2179)
  Revert "Don't custom button props over the bridge (wix#2174)" (wix#2177)
  Don't custom button props over the bridge (wix#2174)
  Set missing TopBar background color
  Register multiple navigatorEventListeners (wix#2173)
  ...
@enahum
Copy link
Copy Markdown

enahum commented Nov 27, 2017

@birkir lets say I have a list of components that onLongPress will do the 3D touch peek&pop and it looks like is working, while doing the peek I can see how it sometimes shows me a peek with the props that I've passed to a previews peek.

for instance:
long press component A -> shows peek of A
long press component B -> shows peek of B
long press component A -> shows peek of B

have you or anyone else seen this?

@enahum
Copy link
Copy Markdown

enahum commented Nov 27, 2017

from the logs

Running application ChannelPeek ({
    initialProps =     {
        channelId = 775qkerugp8cmk7cbx1r1em8sa;
        commandType = Push;
        isPreview = 1;
        navigatorEventID = "screenInstanceID5_events";
        navigatorID = "controllerID1_nav";
        previewViewID = 153;
        screenInstanceID = screenInstanceID5;
        timestamp = 1511806967339;
    };
    rootTag = 31;
})

Running application ChannelPeek ({
    initialProps =     {
        channelId = bkzo93oum38b8p3bzgspyguyyy;
        commandType = Push;
        isPreview = 1;
        navigatorEventID = "screenInstanceID6_events";
        navigatorID = "controllerID1_nav";
        previewViewID = 164;
        screenInstanceID = screenInstanceID6;
        timestamp = 1511806971471;
    };
    rootTag = 41;
})

Running application ChannelPeek ({
    initialProps =     {
        channelId = 775qkerugp8cmk7cbx1r1em8sa;
        commandType = Push;
        isPreview = 1;
        navigatorEventID = "screenInstanceID7_events";
        navigatorID = "controllerID1_nav";
        previewViewID = 153;
        screenInstanceID = screenInstanceID7;
        timestamp = 1511806975328;
    };
    rootTag = 51;
})

first preview = OK
second preview = OK
third preview = FAIL (this one shows the content of the previewViewID = 164 instead of 153

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 27, 2017

Thanks. It seems that If you do a peek and then cancel quickly, the next peeking screen will actually be the top view controller and I think that is what is happening for you.

I will need some help debugging why this is happening, as apple says that the previewing api will handle deallocating the previewing view controller, but doesn't seem to be the case.

Maybe it would work to simply set the previewController to nil after successful push. I'll investigate but any help would be greatly appreciated.

@enahum
Copy link
Copy Markdown

enahum commented Nov 27, 2017

I really don't know much about object-C but I was looking at the docs and I'm wondering if you ever do unregisterForPreviewing?

@enahum
Copy link
Copy Markdown

enahum commented Nov 27, 2017

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 28, 2017

Yes, Apple's docs say:

The system calls this method automatically when a 3D Touch-registered view controller is deallocated.
In some circumstances, you must explicitly call this method. This is the case if a registered view controller’s view hierarchy changes state, or if presenting a preview is otherwise no longer possible. In such cases, call this method.

https://developer.apple.com/documentation/uikit/uiviewcontroller/1621395-unregisterforpreviewing

But yes, we may have to call it manually.

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Nov 28, 2017

@enahum #2239

@niktonic21
Copy link
Copy Markdown

@messense Did you run preview with FlatList? I have same issue. With single page its ok.

@messense
Copy link
Copy Markdown

@niktonic21 Yes, I did run preview with FlatList.

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Dec 12, 2017

@messense @niktonic21 We are still waiting for the fix to be merged. It is ready!

@guyca
Copy link
Copy Markdown
Collaborator

guyca commented Dec 13, 2017

Hey guys, sorry about the delay. Merged now, please ping me on discord if I'm not responsive here 👍

garrettm pushed a commit to Ginger-Labs/react-native-navigation that referenced this pull request Dec 19, 2017
Add the preview and pop feature

To use the feature you simply call `navigator.push()` but with additional parameter `previewViewID` which is a React Node.

```js
<TouchableOpacity
  onPressIn={() => this.props.navigator.push({
    screen: 'screenid',
    previewView: this.viewRef,
    previewActions: [{
      title: 'Foo',
      style: 'selected', // none, selected, destructive
      actions: [{ title: 'Bar' }],
    }],
 })
/>
```
garrettm pushed a commit to Ginger-Labs/react-native-navigation that referenced this pull request Dec 19, 2017
Add the preview and pop feature

To use the feature you simply call `navigator.push()` but with additional parameter `previewViewID` which is a React Node.

```js
<TouchableOpacity
  onPressIn={() => this.props.navigator.push({
    screen: 'screenid',
    previewView: this.viewRef,
    previewActions: [{
      title: 'Foo',
      style: 'selected', // none, selected, destructive
      actions: [{ title: 'Bar' }],
    }],
 })
/>
```
@anonrig
Copy link
Copy Markdown

anonrig commented Dec 29, 2017

Hello, does anybody have a working example of this preview 3d touch feature?

@birkir
Copy link
Copy Markdown
Contributor Author

birkir commented Dec 29, 2017

https://github.com/wix/react-native-navigation/blob/master/example/src/screens/NavigationTypes.js#L132-L138

@charliesbot
Copy link
Copy Markdown

charliesbot commented Dec 30, 2017

Hi, I'm seeing a weird case. if you peek a view several times (without popping it), and then you actually decide to pop a view, it will do the animation, but it won't show the new view. Even if you just press the view, it won't go to the "details" view.
demo

@anonrig
Copy link
Copy Markdown

anonrig commented Dec 30, 2017

@charliesbox I face the same issue. After this issue persists, onPress doesn't get fired at all. So, I can't access the detail page as shown below:

Screenshare

@charliesbot
Copy link
Copy Markdown

@anonrig I opened an issue to track it better

#2445

@anonrig
Copy link
Copy Markdown

anonrig commented Dec 30, 2017

@charliesbox Can you also add my gif animation to the issue?

@charliesbot
Copy link
Copy Markdown

@anonrig sure thing 👍

@harisb2012
Copy link
Copy Markdown

What happened with this guys?

@milesscherrer
Copy link
Copy Markdown

What happened with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.